When I explicitly pass in a schema it's no longer a dictionary. Then this validator fails since data["schema"] is an acutal Schema and cannot be referenced. I'm not really familiar with Pydantic but my understanding of this validator is that it's meant for just converting the schema metadata a common representation that can be shared with V2 metadata. I think that should only happen when reading the actual JSON file.

I'm still working through another issue for partition specs in V1 (this error is thrown in the updated test):

pydantic_core._pydantic_core.ValidationError: 2 validation errors for TableMetadataV1
 partition_spec.0
Input should be a valid dictionary [type=dict_type, input_value=('spec_id', 0), input_type=tuple]
 For further information visit https://errors.pydantic.dev/2.6/v/dict_type
partition_spec.1
 Input should be a valid dictionary [type=dict_type, input_value=('fields', ()), input_type=tuple]
  For further information visit https://errors.pydantic.dev/2.6/v/dict_type

I'm not sure why this happens for V1 metadata explicitly constructed but V2 works fine.

Hey @amogh-jahagirdar Yes, this is a bit awkward in Pydantic. This is a @model_validator(mode="before") which means that it will always apply the validator before trying to construct the model. This will allow you to correct certain things before applying the checks. This particular function will make sure that the schema-id is set. Where schema_id is required. Keep in mind that this also does have applied the aliases, so both schema_id and schema-id are valid.

The before-mode model validator also be applied when you initialize something off: TableMetadataV1(bla=(1,2,3))

The error that you're seeing is after the mode="before" validators are applied. I would put a breakpoint in the validator, and trace up the stack where the bogus data comes from.

Hey @amogh-jahagirdar Yes, this is a bit awkward in Pydantic. This is a @model_validator(mode="before") which means that it will always apply the validator before trying to construct the model. This will allow you to correct certain things before applying the checks. This particular function will make sure that the schema-id is set. Where schema_id is required. Keep in mind that this also does have applied the aliases, so both schema_id and schema-id are valid.

The before-mode model validator also be applied when you initialize something off: TableMetadataV1(bla=(1,2,3))

The error that you're seeing is after the mode="before" validators are applied. I would put a breakpoint in the validator, and trace up the stack where the bogus data comes from.

I'd rather avoid the before validators to patch in the partition-spec and schema (probably sort-order as well?). I'd much rather see that we explicitly pass in the spec below:

    if format_version == 1:
        return TableMetadataV1(
            location=location,
            schema=fresh_schema,
            last_column_id=fresh_schema.highest_field_id,
            current_schema_id=fresh_schema.schema_id,
+           partition_spec=fresh_partition_spec,
            partition_specs=[fresh_partition_spec],
            default_spec_id=fresh_partition_spec.spec_id,
+           sort_order=fresh_sort_order,
            sort_orders=[fresh_sort_order],
            default_sort_order_id=fresh_sort_order.order_id,
            properties=properties,
            last_partition_id=fresh_partition_spec.last_assigned_field_id,
            table_uuid=table_uuid,
        )

My biggest concern is again that this is a before validator, and we don't take into account the aliases (partition-specs, partition-spec-id, etc).

It looks like Java just ignores the schema if there is a schemas: https://github.com/apache/iceberg/blob/5f577f1b9902ffe6181897a439686a31fc81b89a/core/src/main/java/org/apache/iceberg/TableMetadataParser.java#L343-L368

Yeah it just felt a bit awkward to pass in the fields in a redundant way. As long as the source of truth is correct when reading the fields, which it is as you saw for schemas then we're good. I've updated.

Also PartitionSpec on the V1 metadata is a list[dict[str, any]], right now I work with that to preserve compatibility but that does make things a bit awkward since we have to pass in the model_dump of partition spec instead of just the normal PartitionSpec.

Actually @HonahX had a good catch here, it's not just a dump of partition spec, the List[dict[str, any]] is a list of the fields themselves based on the spec. I updated test_metadata. Also, The tests in #245 exercise this path more so going forward so we should have more coverage.